-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refactor indices pallet to use fungible traits #9610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Refactor indices pallet to use fungible traits #9610
Conversation
…s://github.com/Krayt78/polkadot-sdk into Refactor-Indices-pallet-to-use-fungible-traits
|
Asking for reviews to run the bot so i can correct whats missing and also having some insight on if what i am doing is correct or not as this is the first time i am doing a migration so i am pretty sure i am doing something wrong |
…ght handling during account migration steps. Updated function names for clarity and adjusted weight consumption checks.
|
/cmd label T2-pallets |
| return Err(SteppedMigrationError::InsufficientWeight { required: min_required }); | ||
| } | ||
|
|
||
| loop { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it is standard to do a loop inside a step, we could also just do one step which is migration one account or a maximum number of account.
Probably both ways are fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw other migrations doing loops in the step. i am not sure on how these migrations work, is it one step per block ? Cause if it is it would make the migration quite slow to do them 1 by 1.
I could just look at the remaining weight available and calculate how many migrations i can do in that step and remove the loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect they do steps until the block is filled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I don't exactly know how multi step works. but if I was designing it I would have made as many steps are possible in a block, in a way programmable by the block author so that is the step weight is underestimating then it doesn't get stuck but instead the block author can force to decrease the number of steps.
What I am scared about is that we loop to fill the weight but actually our benchmark underestimate the weight and the actual weight is too big for the parachain block, and then the block production is stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after some digging it seems that the framework calls step() once per block.
| let used_weight = <System as frame_system::Config>::MultiBlockMigrator::step(); |
| // A migration cannot progress more than one step per block, we therefore break. |
so to me using a loop iinside the step to empty all the weight available is the correct pattern
…migration benchmark to avoid benchmarking it everytime
…rate for account storage insertion.
Overview
This PR migrates the indices pallet from the Currency trait to the fungible traits with holds, implementing a comprehensive migration strategy that preserves all index relationships and ensures no funds are lost during the transition.
Changes
Implemented MigrateCurrencyToFungibles migration that converts from Currency reserves to Fungibles holds
Migration uses stepped execution to handle large numbers of accounts without timing out
Added detailed logging throughout the migration process for debugging and monitoring
Storage Changes
Updated to use the existing Accounts storage with Fungibles holds
Test Coverage
Updated the new tests to be compatible with the changes
Resources
This is part of the umbrella task #226.
Migration
I have setup a draft migration case but still need to test with some fork of the chain to see how it goes with real life data.